-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Errors #4855
base: master
Are you sure you want to change the base?
Errors #4855
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on your submission! 🎉 While the code is well-structured and effectively handles various interactions, there are a few areas for improvement to enhance functionality and user experience:
- BEM and SCSS Usage: Ensure all class names follow BEM methodology consistently across all files.
- Data Attributes: Add
data-qa="card-hover"
to the "Buy" link inside the first card as specified. - Hover Effects: Consider using a more subtle scale value to prevent layout shifts.
- Dynamic Star Ratings: Implement logic to dynamically render the correct number of filled stars based on the
reviews
value. - Image Handling: Add error handling or a default image for
product.imgSrc
. - Navigation Links: Provide valid URLs or implement JavaScript functions for navigation links and the logo.
- 'Buy' Button: Implement a proper link or event handler for purchasing functionality.
These improvements will help you meet the task requirements and constraints more effectively. Keep up the good work, and continue to learn from these insights! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
@@ -27,7 +27,9 @@ module.exports = async (page, scenario) => { | |||
} | |||
|
|||
if (postInteractionWait) { | |||
await new Promise(resolve => setTimeout(resolve, postInteractionWait)); | |||
await new Promise(resolve => { | |||
setTimeout(resolve, postInteractionWait); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that postInteractionWait
is a positive integer to avoid unexpected behavior in the setTimeout
function.
card.setAttribute('data-qa', 'card'); | ||
|
||
card.innerHTML = ` | ||
<img class="card__img" src="${product.imgSrc}" alt="${product.altText}" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding error handling or a default image in case product.imgSrc
is not a valid URL or the image fails to load.
<span class="stars__star"></span> | ||
<span class="stars__star"></span> | ||
</div> | ||
<p class="card__text-review">Reviews: ${product.reviews}</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The number of stars displayed does not currently reflect the product.reviews
value. Consider implementing logic to dynamically render the correct number of filled stars based on the reviews
value.
<p class="card__text-price">Price:</p> | ||
<p class="card__text-value">${product.price}</p> | ||
</div> | ||
<a href="#" class="card__button">Buy</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'Buy' button currently has a href
attribute set to '#', which doesn't lead anywhere. Consider implementing a proper link or event handler for purchasing functionality.
src/index.html
Outdated
<h1>Catalog</h1> | ||
<header class="header"> | ||
<a | ||
href="#" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The href
attribute for the logo link is set to '#', which doesn't lead anywhere. Consider providing a valid URL or implementing a JavaScript function to handle the click event if navigation is intended.
color: $normal-color; | ||
|
||
&:hover { | ||
color: $active-color; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that $active-color
is defined in the imported variables file. If not, this could lead to a compilation error or unexpected styling.
src/styles/blocks/nav.scss
Outdated
} | ||
|
||
&.is-active { | ||
color: $active-color; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that $active-color
is defined in the imported variables file. If not, this could lead to a compilation error or unexpected styling.
src/styles/blocks/nav.scss
Outdated
} | ||
|
||
.is-active { | ||
color: $active-color; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that $active-color
is defined in the imported variables file. If not, this could lead to a compilation error or unexpected styling.
src/styles/blocks/nav.scss
Outdated
position: absolute; | ||
bottom: 0; | ||
left: 0; | ||
background-color: $active-color; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that $active-color
is defined in the imported variables file. If not, this could lead to a compilation error or unexpected styling.
src/styles/blocks/stars.scss
Outdated
&:nth-child(-n + 4) { | ||
background-image: url(../images/star-active.svg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The :nth-child(-n + 4)
selector will always apply the active star image to the first four stars. If the number of active stars should be dynamic based on a rating value, consider using JavaScript to dynamically apply a class or style to the appropriate number of stars.
No description provided.